Use feature flag to create internal LB #5311

nawazkh · 2024-11-25T17:59:54Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #5296

Special notes for your reviewer:

This PR follows Create and add InternalILB as a CAPZ feature flag #5304

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

feat APIServerILB: create ILB when feature flag APIServerILB is enabled

k8s-ci-robot · 2024-11-25T17:59:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

api/v1beta1/azurecluster_default.go

nawazkh · 2024-11-26T23:40:37Z

/test

k8s-ci-robot · 2024-11-26T23:40:40Z

@nawazkh: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-aks
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-custom-builds
/test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-load-test-custom-builds
/test pull-cluster-api-provider-azure-windows-custom-builds
/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

nawazkh · 2024-11-26T23:44:09Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

codecov · 2024-12-02T21:31:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.53%. Comparing base (983bf8b) to head (0ea69a2).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5311   +/-   ##
=======================================
  Coverage   52.53%   52.53%           
=======================================
  Files         272      272           
  Lines       29421    29422    +1     
=======================================
+ Hits        15456    15457    +1     
  Misses      13166    13166           
  Partials      799      799

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nawazkh · 2024-12-02T22:09:58Z

/test pull-cluster-api-provider-azure-apiversion-upgrade

jackfrancis · 2024-12-03T16:27:09Z

azure/scope/cluster.go

-			&loadbalancers.LBSpec{
-				// API Server LB
-				Name:                 s.APIServerLB().Name,
+		frontendLB := &loadbalancers.LBSpec{


Why are we changing the way we construct the cluster LB in this way? I don't see any functional difference (indeed I wouldn't want to) and I don't see any of these new variables being re-used by the conditional ILB construction below.

You are right, there is no functional difference.

We are using a separate variable for the APIServer LB because the logic to "save" frontendIPs has been pulled to a for loop.
It's been pulled out because the frontendIPs can have at most two IPs.
It seemed cleaner to me to have it as a separate local variable.

I think there is more value in optimizing for a leaner diff.

We aren't making any functional changes to the existing LB constructor outcomes

We are making a significant functional change within this function scope

If possible, we want the diff to be a reflection of the above, making #2 in particular as conspicuous as possible, to predict for better reviews (easier to read) and easier historical analysis ("what exactly did we do with that change?").

If there are other improvements to be made as part of the process of encountering this code surface area, let's capture that with a short description in an issue for working on in the future based on prioritization.

My bad that this diff is causing some friction. This code block should have been part of "making private IP configurable".

Thank you for the input, pulling out the diff.

jackfrancis · 2024-12-03T20:27:16Z

azure/scope/cluster.go

-		})
+		}
+
+		// set the internal IP for the internal LB


is there a reason we're making this change to assign the FrontendIPConfigs outside of the &loadbalancers.LBSpec struct literal?

A side affect of "making private Ip configurable" as part of #5315. Sorry for the confusion.
I have pulled out the above code block in the recent commit. Diff should be cleaner now.

jackfrancis · 2024-12-03T20:57:12Z

azure/scope/cluster.go

@@ -284,7 +284,7 @@ func (s *ClusterScope) LBSpecs() []azure.ResourceSpecGetter {
 			SubnetName:        s.ControlPlaneSubnet().Name,
 			FrontendIPConfigs: []infrav1.FrontendIP{
 				{
-					Name: s.APIServerLB().Name + "-internal-frontEnd", // TODO: improve this name.
+					Name: s.APIServerLB().Name + "-internal-frontEnd",


Because we didn't improve this name we probably can't remove the // TODO :)

Added back the TODO.

jackfrancis · 2024-12-03T22:30:38Z

@nawazkh let's take advantage of this PR to update the file triggers for the apiversion upgrade job

I think anytime we change azure/scope/ (there are probably more directories, but we can worry about that later) we should run that job.

nawazkh · 2024-12-03T23:00:14Z

Please take a look kubernetes/test-infra#33903

jackfrancis · 2024-12-04T00:37:49Z

/lgtm
/approve

k8s-ci-robot · 2024-12-04T00:37:55Z

LGTM label has been added.

Git tree hash: 1442a58f21e72b3344d70c3f32cc3f83ebc5422f

k8s-ci-robot · 2024-12-04T00:37:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from Jont828 and mboersma November 25, 2024 18:00

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 25, 2024

nawazkh force-pushed the create_internal_lb branch from 0340bf6 to f13bec7 Compare November 26, 2024 18:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2024

nawazkh force-pushed the create_internal_lb branch from f13bec7 to 90ba0f0 Compare November 26, 2024 18:53

jackfrancis reviewed Nov 26, 2024

View reviewed changes

api/v1beta1/azurecluster_default.go Outdated Show resolved Hide resolved

nawazkh force-pushed the create_internal_lb branch from 4bba77d to 9f43ff7 Compare November 26, 2024 23:29

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2024

nawazkh force-pushed the create_internal_lb branch from 90ba0f0 to 3536e79 Compare November 26, 2024 23:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 26, 2024

nawazkh changed the title ~~Use feature flag to create internal LB for self hosted templates~~ Use feature flag to create internal LB Nov 26, 2024

nawazkh marked this pull request as ready for review November 26, 2024 23:40

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2024

k8s-ci-robot requested a review from jackfrancis November 26, 2024 23:40

nawazkh mentioned this pull request Nov 26, 2024

[WIP] Make private IP of the internal LB configurable #5315

Closed

4 tasks

nawazkh force-pushed the create_internal_lb branch from 3536e79 to 8d09e90 Compare November 26, 2024 23:50

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2024

nawazkh force-pushed the create_internal_lb branch from faee040 to 1d605fe Compare December 2, 2024 22:07

jackfrancis reviewed Dec 3, 2024

View reviewed changes

nawazkh requested a review from jackfrancis December 3, 2024 17:55

nawazkh force-pushed the create_internal_lb branch 2 times, most recently from dbde3cc to 45d9ad5 Compare December 3, 2024 19:57

jackfrancis reviewed Dec 3, 2024

View reviewed changes

nawazkh force-pushed the create_internal_lb branch from 45d9ad5 to 9e35b02 Compare December 3, 2024 20:39

nawazkh requested a review from jackfrancis December 3, 2024 20:44

jackfrancis reviewed Dec 3, 2024

View reviewed changes

use feature flag to set default ILB

0ea69a2

nawazkh force-pushed the create_internal_lb branch from 9e35b02 to 0ea69a2 Compare December 3, 2024 21:16

nawazkh requested a review from jackfrancis December 3, 2024 21:18

nawazkh mentioned this pull request Dec 3, 2024

CAPZ: Trigger api version upgrade scope changes kubernetes/test-infra#33903

Merged

k8s-ci-robot assigned jackfrancis Dec 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2024

k8s-ci-robot merged commit 1275143 into kubernetes-sigs:main Dec 4, 2024
21 checks passed

k8s-ci-robot added this to the v1.18 milestone Dec 4, 2024

nawazkh deleted the create_internal_lb branch December 4, 2024 00:47

nawazkh mentioned this pull request Dec 12, 2024

let users configure private IP of the internal LB #5332

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use feature flag to create internal LB #5311

Use feature flag to create internal LB #5311

nawazkh commented Nov 25, 2024 •

edited

Loading

k8s-ci-robot commented Nov 25, 2024

nawazkh commented Nov 26, 2024

k8s-ci-robot commented Nov 26, 2024

nawazkh commented Nov 26, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

nawazkh commented Dec 2, 2024

jackfrancis Dec 3, 2024

nawazkh Dec 3, 2024 •

edited

Loading

jackfrancis Dec 3, 2024

nawazkh Dec 3, 2024

jackfrancis Dec 3, 2024

nawazkh Dec 3, 2024 •

edited

Loading

jackfrancis Dec 3, 2024

nawazkh Dec 3, 2024

nawazkh Dec 3, 2024

jackfrancis commented Dec 3, 2024

nawazkh commented Dec 3, 2024

jackfrancis commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

Use feature flag to create internal LB #5311

Use feature flag to create internal LB #5311

Conversation

nawazkh commented Nov 25, 2024 • edited Loading

k8s-ci-robot commented Nov 25, 2024

nawazkh commented Nov 26, 2024

k8s-ci-robot commented Nov 26, 2024

nawazkh commented Nov 26, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

nawazkh commented Dec 2, 2024

jackfrancis Dec 3, 2024

Choose a reason for hiding this comment

nawazkh Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

jackfrancis Dec 3, 2024

Choose a reason for hiding this comment

nawazkh Dec 3, 2024

Choose a reason for hiding this comment

jackfrancis Dec 3, 2024

Choose a reason for hiding this comment

nawazkh Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

jackfrancis Dec 3, 2024

Choose a reason for hiding this comment

nawazkh Dec 3, 2024

Choose a reason for hiding this comment

nawazkh Dec 3, 2024

Choose a reason for hiding this comment

jackfrancis commented Dec 3, 2024

nawazkh commented Dec 3, 2024

jackfrancis commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

nawazkh commented Nov 25, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

nawazkh Dec 3, 2024 •

edited

Loading

nawazkh Dec 3, 2024 •

edited

Loading